Fix swap-deps to handle missing reference directories gracefully #64
Conversation
Previously, swap-deps would fail with an error if a local path reference in .swap-deps.yml or specified on the command line didn't exist. This was problematic in Conductor workflows where workspace directories and branches are frequently deleted after merging. Changes: - Modified validate_local_paths! to show warnings instead of raising errors for missing directories - Updated swap_gemfile, swap_package_json, and build_local_packages! to skip missing paths with informative messages - GitHub-managed repos are excluded from validation (they're cloned on demand) - Updated tests to reflect new warning behavior instead of error raising - Fixed test stubs to allow additional File.exist? calls for git branch detection This allows the tool to gracefully handle stale references and continue processing other valid dependencies. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Walkthroughvalidate_local_paths! now collects missing local gem/npm paths and emits a consolidated non-fatal warning instead of raising. Subsequent swap/build flows skip missing or GitHub-managed items, record skipped gems, and emit per-item notices; specs updated to expect warnings, "Dependencies:" labeling, and expanded re-swap/branch/ref handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GemSwapper
participant FileSystem
rect rgb(200,220,255)
note over GemSwapper: Old Flow — hard failure on missing local paths
User->>GemSwapper: validate_local_paths!
GemSwapper->>FileSystem: check gem/npm paths
FileSystem-->>GemSwapper: some paths missing
GemSwapper->>GemSwapper: raise DemoScripts::Error
GemSwapper-->>User: ❌ Exception
end
rect rgb(220,255,220)
note over GemSwapper: New Flow — collect, warn, skip missing paths
User->>GemSwapper: validate_local_paths!
GemSwapper->>FileSystem: check gem/npm paths
FileSystem-->>GemSwapper: some paths missing
GemSwapper->>GemSwapper: collect missing paths
GemSwapper-->>User: ⚠️ Consolidated warning (missing paths)
User->>GemSwapper: swap_gemfile / swap_package_json / build_local_packages!
loop per dependency
GemSwapper->>FileSystem: check dependency path
alt path exists
GemSwapper->>GemSwapper: perform swap/build
else path missing
GemSwapper-->>User: 📌 Skip notice (recorded in @skipped_gems)
end
end
GemSwapper-->>User: ✅ print_swap_summary (swapped vs skipped)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (1)spec/demo_scripts/gem_swapper_spec.rb (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (16)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/demo_scripts/gem_swapper.rb (1)
1308-1312: Consider including path in skip message for consistency.The skip logic correctly prevents build attempts on missing paths. The message format is slightly inconsistent with
swap_gemfile(line 916) andswap_package_json(line 1014), which both include the full path. Including it here would maintain consistency and provide more context.Apply this diff for consistency:
- puts " ⊘ Skipping #{gem_name} (path does not exist)" + puts " ⊘ Skipping #{gem_name} (path does not exist: #{local_path})"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/demo_scripts/gem_swapper.rb(8 hunks)spec/demo_scripts/gem_swapper_spec.rb(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
spec/demo_scripts/gem_swapper_spec.rb (1)
lib/demo_scripts/gem_swapper.rb (1)
include(14-1379)
🔇 Additional comments (6)
lib/demo_scripts/gem_swapper.rb (3)
813-836: Well-designed graceful degradation.The refactoring from hard failure to warning-based validation is well-executed. Collecting all missing paths upfront and providing clear remediation guidance (update .swap-deps.yml or use --restore) improves the user experience. Excluding GitHub-managed repos from validation (line 820) is correct since they're cloned on demand in
clone_github_repos!.
914-918: Appropriate skip-on-missing behavior.The directory check and skip logic correctly handles missing paths during the swap operation. This provides a second layer of validation beyond
validate_local_paths!, which is important for TOCTOU scenarios where a directory might be deleted between validation and swap.
1012-1016: Consistent npm package handling.The skip logic mirrors
swap_gemfile, maintaining consistency across gem and npm package swapping operations.spec/demo_scripts/gem_swapper_spec.rb (3)
252-259: Test correctly validates non-fatal warning behavior.The updated test properly verifies that
validate_local_paths!now emits a warning to stdout instead of raising an error when paths are missing.
771-771: Updated to match refined error message.The test expectation correctly reflects the updated warning message in the implementation.
911-1050: Test updates properly reflect UI improvements.The test changes correctly validate:
- Default mocks for
File.exist?(lines 911, 970, 989) improve test isolation- Updated status output heading from "Gemfile:" to "Dependencies:" (lines 923, 942, 1012)
- Enhanced GitHub repo display with branch info and repository badges (lines 943-946, 962-964)
- Refined messages distinguishing between "No swapped dependencies" and "No currently swapped dependencies (backups available)"
These changes align well with the implementation improvements in the main file.
Code Review for PR #64: Fix swap-deps to handle missing reference directories gracefullySummaryThis PR improves the swap-deps tool by converting missing directory errors into warnings, allowing graceful handling of deleted Conductor workspaces or stale references. The changes are well-structured and address a real pain point in the workflow. Strengths1. Well-Designed Solution
2. Good Error Handling
3. Comprehensive Test Updates
4. Code Quality
Suggestions and Considerations1. Potential Logic Issue in validate_local_paths!Location: lib/demo_scripts/gem_swapper.rb:820 Question: Is there a scenario where a gem could be in both gem_paths (with a local path) AND github_repos? If so, which takes precedence? Consider adding a comment clarifying the precedence. 2. Duplicate Directory ChecksAfter validate_local_paths! warns about missing paths, three other methods re-check File.directory? and print skip messages. Users may see duplicate messages. Consider tracking warned paths to suppress redundant skip messages, or keep as-is for clarity (acceptable). 3. Test Coverage GapMissing test case: Verify the interaction between validate_local_paths! and subsequent skip behavior. Current tests check them independently but not together. 4. Minor: Inconsistent Skip MessagesThe skip messages have slight variations across methods. Consider standardizing the format for consistency. SecurityNo security concerns identified. The changes maintain existing validation and don't introduce new risks. Final RecommendationAPPROVE with minor suggestions. This is a solid PR that solves a real problem effectively. The suggestions above are minor improvements that could be addressed in a follow-up if desired. The core functionality is sound, well-tested, and follows the repository's conventions. Great work on improving the developer experience! |
Previously, swap-deps would skip gems that were already swapped (had path: or github: in the Gemfile). This prevented updating to a different local path or switching between branches after a branch was deleted. Changes: - Modified swap_gem_in_gemfile to replace existing path: or github: references instead of skipping them - Modified swap_gem_to_github to replace existing path:, github:, branch:, and tag: references - Both methods now preserve other gem options (like require: false) when re-swapping - Added comprehensive tests for re-swapping scenarios: * Path to different path * GitHub branch to different branch * Path to GitHub branch * GitHub branch to path * Preserving options during re-swap This enables workflows where developers switch between different Conductor workspace directories or GitHub branches, even when the previous reference no longer exists. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Pull Request Review - PR #64SummaryThis PR improves the ✅ StrengthsCode Quality
Test Coverage
User Experience
🔍 Detailed Analysis1. Code Quality and Best Practicesvalidate_local_paths! (lib/demo_scripts/gem_swapper.rb:813-837)
swap_gem_in_gemfile and swap_gem_to_github (lib/demo_scripts/gem_swapper.rb:945-1005)
2. Potential Bugs or IssuesMissing path handling
Regex patterns
3. Performance Considerations
4. Security Concerns
5. Test CoverageNew test cases (spec/demo_scripts/gem_swapper_spec.rb:101-135)
📋 RecommendationsMedium Priority
🎯 ConclusionRecommendation: ✅ APPROVE This is a high-quality PR that:
The code is well-structured, the tests are comprehensive, and the implementation is robust. The suggestions above are optional improvements. Great work, @justin808! This will significantly improve the developer experience when working with Conductor workspaces. 🤖 Generated with Claude Code |
Changes:
- Added clarifying comments about github_repos precedence in validate_local_paths!
GitHub repos take precedence and overwrite gem_paths entries after cloning
- Standardized skip message format across all methods:
* swap_gemfile: "⊘ Skipping {gem} - path does not exist: {path}"
* swap_package_json: "⊘ Skipping {gem} npm package - path does not exist: {path}"
* build_local_packages!: "⊘ Skipping {gem} build - path does not exist: {path}"
- Added integration test verifying validate_local_paths! warns and subsequent
swap operations skip with appropriate messages
- Updated comments to clarify that users are warned BEFORE skip messages appear
All 91 tests pass.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
Pull Request ReviewSummaryThis PR successfully transforms ✅ Strengths1. Well-Motivated ChangesThe PR description clearly articulates the problem (Conductor workflow issues with deleted workspaces) and provides a comprehensive solution. The dual improvements (graceful missing path handling + re-swapping support) work well together. 2. Excellent Test Coverage
3. Consistent ImplementationThe skip-on-missing-path pattern is consistently applied across 4. Good User Experience
🔍 Code Quality & Best PracticesPositive Observations:
Minor Concerns:1. Regex Complexity in Option StrippingThe option extraction logic uses sequential Suggestion: Consider extracting duplicate option-stripping logic to a helper method (it appears in both 2. Output ConsistencyThe skip messages use 🐛 Potential Bugs/Issues1. Warning-Then-Skip Gap (Minor)There's a small theoretical race condition if a directory is deleted between Impact: Low - unlikely in practice, and worst case is a file read error which Ruby handles gracefully. ⚡ Performance ConsiderationsNo Performance Regression: The changes add minimal overhead
🔒 Security Concerns✅ No Security Issues Identified
📋 Test Coverage AssessmentExcellent Coverage:
Suggestions for Additional Tests:
📊 Overall Assessment
🎯 RecommendationsRequired Before Merge:
Nice-to-Have Improvements:
✅ Final VerdictAPPROVED - This PR successfully achieves its goals with solid implementation and excellent test coverage. The transformation from fail-fast to graceful degradation is exactly what Conductor workflows need. The re-swapping feature is implemented correctly and thoroughly tested. The minor suggestions above are optimizations, not blockers. Great work! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/demo_scripts/gem_swapper.rb (2)
949-968: Bundler breakage risk: don’t leavegit:/ref:options when swapping to path.If the original line had
git:orref:, the current regex doesn’t remove them. Those options are invalid forpath:sources and can cause Gemfile parsing errors. Strip them alongsidegithub/branch/tag.Apply this diff in
swap_gem_in_gemfile:options = rest.dup options = options.sub(/^\s*,\s*(['"])[^'"]*\1/, '') # Remove version if present options = options.sub(/,\s*path:\s*(['"])[^'"]*\1/, '') # Remove path: if present + options = options.sub(/,\s*git:\s*(['"])[^'"]*\1/, '') # Remove git: if present options = options.sub(/,\s*github:\s*(['"])[^'"]*\1/, '') # Remove github: if present options = options.sub(/,\s*branch:\s*(['"])[^'"]*\1/, '') # Remove branch: if present options = options.sub(/,\s*tag:\s*(['"])[^'"]*\1/, '') # Remove tag: if present + options = options.sub(/,\s*ref:\s*(['"])[^'"]*\1/, '') # Remove ref: if present
976-1004: Prevent conflicting options: stripgit:/ref:when swapping to GitHub.If the source had
git:orref:they remain, yielding conflicting params likegithub + branch + ref. Remove them like others.Apply this diff in
swap_gem_to_github:options = rest.dup options = options.sub(/^\s*,\s*(['"])[^'"]*\1/, '') # Remove version if present options = options.sub(/,\s*path:\s*(['"])[^'"]*\1/, '') # Remove path: if present + options = options.sub(/,\s*git:\s*(['"])[^'"]*\1/, '') # Remove git: if present options = options.sub(/,\s*github:\s*(['"])[^'"]*\1/, '') # Remove github: if present options = options.sub(/,\s*branch:\s*(['"])[^'"]*\1/, '') # Remove branch: if present options = options.sub(/,\s*tag:\s*(['"])[^'"]*\1/, '') # Remove tag: if present + options = options.sub(/,\s*ref:\s*(['"])[^'"]*\1/, '') # Remove ref: if present
🧹 Nitpick comments (3)
lib/demo_scripts/gem_swapper.rb (2)
1021-1025: Good: skip missing local paths for npm swaps; consider parse guard.Behavior matches CLI warnings. Optional: rescue
JSON::ParserErrorhere (likedetect_swapped_packages) to avoid aborting on malformed package.json.Example:
data = JSON.parse(File.read(package_json_path)) + rescue JSON::ParserError + warn " ⚠️ Warning: Could not parse #{File.basename(package_json_path)}; skipping npm swap" + return
386-413: Outdated-check on tags likely unreliable.
check_outdated_repostreats Git tags like branches, fetchingorigin/<tag>and diffing commits. That often fails or is meaningless for immutable tags.
- Detect
tagvsbranchwhen parsing Gemfile (capture param name), storeref_type, and skip commit-behind checks for tags or display “n/a”.spec/demo_scripts/gem_swapper_spec.rb (1)
44-153: Add tests to catchgit:/ref:leftovers in Gemfile rewrites.To guard against the critical issue flagged in the implementation, add cases ensuring these options are removed.
Example additions:
+ context 'when swapped from github with ref (commit)' do + let(:gemfile_content) { "gem 'shakapacker', github: 'user/repo', ref: 'abcdef'\n" } + it 'removes ref when converting to path' do + result = swapper.send(:swap_gem_in_gemfile, gemfile_content, 'shakapacker', local_path) + expect(result).to eq("gem 'shakapacker', path: '/Users/test/dev/shakapacker'\n") + end + end + + context 'when converting from git url with ref' do + let(:gemfile_content) { "gem 'shakapacker', git: 'https://github.com/shakacode/shakapacker.git', ref: 'abcdef'\n" } + let(:github_info) { { repo: 'shakacode/shakapacker', branch: 'main' } } + it 'removes git/ref and uses github syntax' do + result = swapper.send(:swap_gem_to_github, gemfile_content, 'shakapacker', github_info) + expect(result).to eq("gem 'shakapacker', github: 'shakacode/shakapacker'\n") + end + endAlso applies to: 155-246
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/demo_scripts/gem_swapper.rb(9 hunks)spec/demo_scripts/gem_swapper_spec.rb(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
spec/demo_scripts/gem_swapper_spec.rb (1)
lib/demo_scripts/gem_swapper.rb (1)
include(14-1388)
🔇 Additional comments (8)
lib/demo_scripts/gem_swapper.rb (3)
814-836: Graceful missing-path handling looks good.Consolidated warning and non-fatal flow matches the PR intent. Skipping GitHub-managed repos avoids false positives.
914-919: Good: skip missing local paths in Gemfile swap.Prevents crashes and keeps the run productive with clear notices.
1317-1321: Good: skip missing paths before building local packages.Prevents needless npm work and noisy failures.
spec/demo_scripts/gem_swapper_spec.rb (5)
104-107: Re-swap coverage for GitHub→path and path→path looks solid.These tests pin the intended idempotent rewrite behavior and option preservation.
Also applies to: 113-117, 119-126, 128-134
227-235: Re-swap to GitHub with branch updates is well covered.Confirms branch replacement semantics and formatting.
Also applies to: 237-245
290-297: Warning-not-error expectation is correct for missing local paths.Matches the new non-fatal flow.
947-964: Status output expectations updated appropriately.Checks for “Dependencies:” header, GitHub badges, branch annotations, and backup summaries all align with the new UX.
Also applies to: 978-984, 1000-1003, 1019-1021, 1026-1056, 1068-1071
801-814: Restore path failure warnings validated.Covers both the lock backup restoration and explicit “Check Gemfile for errors” hint.
|
@claude review this PR |
|
Claude finished @justin808's task —— View job Code Review for PR #64Tasks
SummaryThis PR successfully transforms the Strengths1. Well-Designed Solution ✅
2. Comprehensive Implementation ✅
3. Excellent Test Coverage ✅
4. Code Quality ✅
Issues & Recommendations🔴 Critical: Missing
|
| Priority | Issue | Action Required |
|---|---|---|
| 🔴 Critical | Missing git: and ref: option stripping |
Add two more .sub() calls in both swap methods |
| Missing test coverage for git/ref scenarios | Add 2-3 test cases | |
| 💡 Optional | Duplicate skip messages | Consider tracking warned paths (low priority) |
Final Verdict
Recommendation: ✅ APPROVE with required fix
This PR delivers significant value by solving real workflow problems. The implementation is solid, test coverage is comprehensive, and the code quality is high.
Before merging: Please add the git: and ref: option stripping to prevent Bundler errors. This is a quick 2-line fix in each method.
After merging: Consider adding the missing test cases for git/ref scenarios to prevent regressions.
Great work on improving the developer experience! The re-swapping feature and graceful missing path handling will be very useful. 🎉
Problem: When swapping from gems using git: urls (e.g., gem 'name', git: 'url', ref: 'sha'), the git: and ref: parameters were not being removed, resulting in invalid Gemfile entries like: gem 'name', path: '/path', git: 'url', ref: 'sha' This would cause Bundler parsing errors. Changes: - Added git: and ref: removal to swap_gem_in_gemfile regex substitutions - Added git: and ref: removal to swap_gem_to_github regex substitutions - Updated comments to reflect all source specification parameters are removed - Added 7 comprehensive test cases: * Swapping from git: url with ref: to path * Swapping from git: url without ref: to path * Swapping from git: url with branch: to path * Swapping from git: url with ref: and options to path (preserving options) * Swapping from git: url with ref: to github * Swapping from git: url with branch: to github * Swapping from git: url with ref: and options to github (preserving options) All 98 tests pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
PR Review: Fix swap-deps to handle missing reference directories gracefullyOverall AssessmentThis is a well-implemented PR that addresses real-world pain points in the swap-deps workflow. The changes are well-tested, thoughtfully designed, and include excellent documentation. The code quality is high with proper RuboCop compliance and comprehensive test coverage. Code Quality & Best Practices ✅Strengths:
Best Practices Followed:
Potential Issues & Recommendations 🔍1. Regex Duplication (Minor)The option-stripping regex in swap_gem_in_gemfile and swap_gem_to_github are identical. Consider extracting to a helper method to reduce maintenance burden. 2. Silent Skip Behavior (Design Question)When paths are missing, the tool skips them after initial warning. Consider adding a summary at the end showing what was swapped vs. skipped to make this more visible. 3. Edge Case: Empty Options String (Low Risk)If options becomes just whitespace after stripping, you might end up with trailing commas. Consider trimming the final options string. 4. Test Integration Gap (Minor)The integration test validates validation to skip flow nicely, but doesn't verify that valid paths are still processed correctly when some paths are missing. Consider adding a test with mixed valid/invalid paths. Performance Considerations ⚡
Security Considerations 🔒
Test Coverage ✅Excellent coverage with tests for:
Minor gaps: Mixed valid/invalid path scenarios, npm package re-swapping Final VerdictApprove with minor suggestions. This PR successfully solves the stated problems with a clean implementation and strong test coverage. The suggested improvements are minor and don't block merging. Great work! 🎉 |
1. Extract Duplicate Regex Stripping (DRY Principle)
- Created strip_gem_source_params helper method to eliminate code duplication
- Removes: version, path, github, git, branch, tag, ref parameters
- Returns empty string if only whitespace remains, or options with leading comma
- Fixes edge case: trailing commas from empty options after stripping
2. Add Swap Summary Output
- Tracks skipped gems in @skipped_gems array
- New print_swap_summary method shows:
* Number of successfully swapped gems (with names)
* Number of skipped gems due to missing paths (with names)
- Makes silent skip behavior more visible to users
3. Comprehensive Integration Test
- Added test for mixed valid/invalid paths scenario
- Verifies:
* Validation warns only about invalid paths
* Valid paths are processed correctly
* Invalid paths are skipped with appropriate messages
* Gemfile content reflects only valid swaps
All 99 tests pass.
Benefits:
- Reduced maintenance burden (single source of truth for regex patterns)
- Better user visibility into what was swapped vs skipped
- More robust test coverage for real-world scenarios
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
Pull Request ReviewOverall Assessment ✅This is a well-executed PR that successfully addresses the problem of handling missing local paths in the swap-deps tool. The changes are thoughtful, well-tested, and improve the developer experience significantly. Code Quality & Best Practices ⭐Strengths:
Suggestions:
Potential Bugs or Issues 🔍Minor Concerns:
Performance Considerations ⚡Good:
Potential Optimizations:
Security Considerations 🔒Strengths:
No security concerns identified - The changes don't introduce any new security vulnerabilities. Test Coverage 🧪Excellent test coverage! The PR increases test count from 86 to 90 tests and covers:
Test Quality:
Minor Suggestion: SummaryApproval Status: ✅ Approved This PR demonstrates excellent software engineering practices:
The suggestions above are minor optimizations and defensive programming practices that could be addressed in a follow-up PR if desired. They don't block this PR from merging. Recommendation: Merge after CI passes. Great work on improving the developer experience for Conductor workflows! 🎉 Review generated with Claude Code |
Summary
validate_local_paths!to show warnings instead of raising errors for missing directoriesswap_gemfile,swap_package_json, andbuild_local_packages!to skip missing paths with informative messagesswap_gem_in_gemfileandswap_gem_to_githubto allow re-swapping between different paths and branchesProblem
swap-depswould fail with an error if a local path reference in.swap-deps.ymlor specified on the command line didn't exist. This was problematic in Conductor workflows where workspace directories and branches are frequently deleted after merging.Once a gem was swapped to a local path or GitHub branch, it couldn't be re-swapped to a different path/branch without first running
--restore. This was inconvenient when switching between different Conductor workspaces or GitHub branches.Solution
Graceful handling of missing directories
The tool now gracefully handles missing directories by:
Re-swapping support
The tool now supports re-swapping by:
path:references with new pathsgithub:andbranch:references with new onespath:togithub:and vice versarequire: false) during re-swapThis enables workflows where developers can:
--restorefirstTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
UI / Status
Tests